feat: add the "valid-license" rule #786

xenobytezero · 2025-01-28T18:02:06Z

PR Checklist

Addresses an existing open issue: fixes 🚀 Feature: Bring in a valid-license rule #830
That issue was marked as status: accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Adds a new rule that enforces the 'licence' value in the package.json to be a specific value, or one of a selection of values.

Proposing this as a new rule to eliminate my usage of npm-package-json-lint, since this was the only feature I needed from it.

I did have this as a custom rule in my own ESLint plugin, but I found out it's very difficult to apply multiple plugins to the same file and I am already using this plugin.

This rule could expand in scope to allow for any value to be validated in the same way.

JoshuaKGoldberg

Thanks for sending this! This was a good prod to close out the conversation in #51 about how we want to approach all these requirements. I posted #51 (comment).

Left a couple of initial review comments here in case you feel motivated to work on this before we're certain this is the direction for the package. Cheers!

src/plugin.ts

src/tests/rules/requires-license.test.ts

michaelfaith

It looks like the rule and rule test files aren't named consistently with the rule itself requires-license vs require-license

xenobytezero · 2025-01-29T10:39:04Z

It looks like the rule and rule test files aren't named consistently with the rule itself requires-license vs require-license

Rookie error, fixed.

michaelfaith · 2025-02-01T17:35:31Z

Thanks for your patience as we worked out the long-term strategy for the rules in this plugin. What we landed on in #51 is that we want to have separate rules for requiring a property and for validating the correctness of that property (if it exists). That way people can use one or the other, or both.

So for require-licence, the rule should just purely test for the existence of the property. And a separate rule valid-licence would check that if the property exists that it is valid. The current state of this PR has both requiredness and validation checks. So, in order for this to land, it would need to be reduced to just the check for requiredness. Does that make sense?

michaelfaith · 2025-02-01T23:47:22Z

Actually, on second though, why don't we convert this one into the valid-license rule, since most of the logic is oriented towards that? The main difference now, though, is that there's no check for required-ness, since the other rule will handle that. Let me know if any of that is unclear and I'll help clear things up.

codecov · 2025-02-02T17:26:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (4c72153) to head (47ccb5a).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
+ Coverage   98.71%   98.78%   +0.07%     
==========================================
  Files          19       20       +1     
  Lines        1163     1234      +71     
  Branches      139      146       +7     
==========================================
+ Hits         1148     1219      +71     
  Misses         15       15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xenobytezero · 2025-02-02T18:09:26Z

Actually, on second though, why don't we convert this one into the valid-license rule, since most of the logic is oriented towards that? The main difference now, though, is that there's no check for required-ness, since the other rule will handle that. Let me know if any of that is unclear and I'll help clear things up.

No problem, can rework this rule to check for validity.

Obviously checking validity would require existence of the property in order to validate the value, and from what I can gather in reading Bring in a "require-*"/"no-*" kind of rule #51 the rule should also fail for non-existance?
Should it fail in the way it does now?
Should I split out the require logic into it's own thing since most of the requires rules will be very similar?

michaelfaith · 2025-02-02T18:13:28Z

and from what I can gather in reading Bring in a "require-"/"no-" kind of rule #51 the rule should also fail for non-existance?

Actually that went back and forth a couple of times (my bad 😅). Where we landed is that valid- rules will not fail if the property isn't present. That will be the responsibility of the require- rules. valid- rules will just validate that, if the property is present, its value is valid. It won't do anything if there isn't a property with that name.

michaelfaith · 2025-02-02T18:14:53Z

Should I split out the require logic into it's own thing since most of the requires rules will be very similar?

I already put up a PR that lays the groundwork for our approach to require- flavor rules: #851 Definitely welcome your input!

xenobytezero · 2025-02-03T11:43:30Z

Just working on this now, looks like the changes would be essentially cosmetic to match what was discussed.

The rule currently has a check that if the property is present that it also must be a string. I don't think there is anything in the current discussion about where type checking of the value should live in require- or valid-?

michaelfaith · 2025-02-03T18:29:31Z

The require- rules won't do any checking of the value. It'll just simply check whether the property is presentor not.

- Added extra valid test case for missing property - Fix documentation - Fix linting errors

JoshuaKGoldberg

Progress 🚀

JoshuaKGoldberg · 2025-02-16T21:03:15Z

src/rules/valid-license.ts

+			invalidValue: '"license" must be{{multiple}} {{allowed}}"',
+			nonString: '"license" must be a string"',
+		},
+		schema: {


[Feature] If this rule is to be enabled in the recommended config -which would be great!- then we're going to need it to not be prescriptive on which of the "valid" license(s) are allowed. Any "valid" license should be allowed unless the user has customized the rule. "Valid" here could mean one of several things, depending on how strict the user wants to be. In increasing order of strictness...

🍏 Any string value

🍌 Any SPDX license ID (on npm: spdx-license-ids) or SPDX license expression syntax referring to SPDX license IDs

🌸 The same as 2., but only OSI approved license IDs

🐉 The same as 2., but only FSF Free/Libre approved license IDs

🥚 >=1 specific license IDs (what the PR has right now)

I think any of 2 🍌, 3 🌸, 4 🐉, and 5 🥚 would be fine as followup issues (with the caveat that 3 🌸 and 4 🐉 depend on 2 🍌). Most users would be satisfied with just 1 🍏 checking that it's a string.

But, the options right now don't allow 1 🍏. They require 5 🥚, passing in a specific list of IDs. Could you please make the 5 🥚 specific license ID checking optional? As in, if the user doesn't provide any licenses, the rule should only check 1 🍏 that it's a valid string.

Reference: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#license

JoshuaKGoldberg · 2025-02-16T21:06:11Z

src/tests/rules/valid-license.test.ts

+
+ruleTester.run("valid-license", rule, {
+    invalid: [
+        // Invalid with single valid value


[Docs] Nit: IMO these comments aren't worth keeping up-to-date. ESLint rule tests tend to get very nuanced and tricky over time. I've seen suits with many dozens or even hundreds of tests that exercise various edge cases on themes. Personally I'd remove this:

Suggested change

// Invalid with single valid value

If you really want to include a name per test, the proper way would be with a name property per https://eslint.org/docs/latest/integrate/nodejs-api#ruletester.

Not a blocker, just a preference. 🙂

src/plugin.ts

JoshuaKGoldberg requested changes Jan 28, 2025

View reviewed changes

src/plugin.ts Outdated Show resolved Hide resolved

src/tests/rules/requires-license.test.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Jan 28, 2025

JoshuaKGoldberg changed the title ~~Add the "require-licence" rule~~ feat: add the "require-license" rule Jan 28, 2025

michaelfaith reviewed Jan 28, 2025

View reviewed changes

xenobytezero force-pushed the require-licence-rule branch from d97ae03 to 3b6a152 Compare January 29, 2025 10:37

Add the "require-licence" rule

7019c1b

xenobytezero force-pushed the require-licence-rule branch from 3b6a152 to 7019c1b Compare January 29, 2025 10:40

xenobytezero requested a review from JoshuaKGoldberg January 29, 2025 17:37

github-actions bot removed the status: waiting for author Needs an action taken by the original poster label Jan 29, 2025

michaelfaith mentioned this pull request Jan 30, 2025

Bring in a "require-*"/"no-*" kind of rule #51

Closed

Merge branch 'main' into require-licence-rule

47ccb5a

michaelfaith mentioned this pull request Feb 1, 2025

🚀 Feature: Bring in a require-license rule #846

Open

3 tasks

michaelfaith added the status: waiting for author Needs an action taken by the original poster label Feb 1, 2025

michaelfaith changed the title ~~feat: add the "require-license" rule~~ feat: add the "valid-license" rule Feb 2, 2025

- Renamed rule to valid-license,

84289f4

- Added extra valid test case for missing property - Fix documentation - Fix linting errors

xenobytezero force-pushed the require-licence-rule branch from bfa389f to 84289f4 Compare February 5, 2025 17:11

JoshuaKGoldberg requested changes Feb 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add the "valid-license" rule #786

feat: add the "valid-license" rule #786

xenobytezero commented Jan 28, 2025 •

edited by michaelfaith

Loading

JoshuaKGoldberg left a comment

michaelfaith left a comment

xenobytezero commented Jan 29, 2025

michaelfaith commented Feb 1, 2025 •

edited

Loading

michaelfaith commented Feb 1, 2025

codecov bot commented Feb 2, 2025

xenobytezero commented Feb 2, 2025

michaelfaith commented Feb 2, 2025

michaelfaith commented Feb 2, 2025

xenobytezero commented Feb 3, 2025

michaelfaith commented Feb 3, 2025

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 16, 2025 •

edited

Loading

JoshuaKGoldberg Feb 16, 2025

feat: add the "valid-license" rule #786

Are you sure you want to change the base?

feat: add the "valid-license" rule #786

Conversation

xenobytezero commented Jan 28, 2025 • edited by michaelfaith Loading

PR Checklist

Overview

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

michaelfaith left a comment

Choose a reason for hiding this comment

xenobytezero commented Jan 29, 2025

michaelfaith commented Feb 1, 2025 • edited Loading

michaelfaith commented Feb 1, 2025

codecov bot commented Feb 2, 2025

Codecov Report

xenobytezero commented Feb 2, 2025

michaelfaith commented Feb 2, 2025

michaelfaith commented Feb 2, 2025

xenobytezero commented Feb 3, 2025

michaelfaith commented Feb 3, 2025

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 16, 2025 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 16, 2025

Choose a reason for hiding this comment

xenobytezero commented Jan 28, 2025 •

edited by michaelfaith

Loading

michaelfaith commented Feb 1, 2025 •

edited

Loading

JoshuaKGoldberg Feb 16, 2025 •

edited

Loading